rbac: add counters to config dump#1536
Conversation
Goes with istio/ztunnel#1536 ``` NAMESPACE POLICY NAME ACTION SCOPE HITS MISSES default httpbin Allow Namespace 7 3 istio-system istio_converted_static_strict Deny WorkloadSelector 2 10 ```
| pub struct AuthzCounter { | ||
| hits: AtomicU64, | ||
| misses: AtomicU64, | ||
| } |
There was a problem hiding this comment.
hit and miss need to be understood relative to the kind of policy to make sense. hit a deny == denied. hit an allow == allowed. Would users be better served with "allows" and "denies" which inform them of a concrete result taken in association with this policy?
There was a problem hiding this comment.
I worry an allow policy with deny: 123 looks alarming. Its not really a deny, its more like it failed to allow
There was a problem hiding this comment.
So if a policy doesn't match, miss/deny will be incremented, even if another policy later on allow or denies it?
There was a problem hiding this comment.
I also wonder if the order of operations might cause the stats to be confusing. For example, if there are multiple deny policies, but only one of them matches, then the order in which the deny policies are evaluated will affect the stats (assuming there is some short circuiting), which seems weird.
There was a problem hiding this comment.
Maybe a better say of saying this is, why do we need miss when we can just do total_requests - hit
There was a problem hiding this comment.
there is no total request counter for a policy
There was a problem hiding this comment.
I mean total request counter across for ztunnel in general
There was a problem hiding this comment.
That doesn't exist is the first issue. We could add it but it doesn't help the use case.
Miss means "policy applies to the request but does not match"
There was a problem hiding this comment.
Ok then only my second comment then: This doesn't seem to take into account short circuiting. Just because a policy could apply, short circuiting might mean that a counter doesn't get incremented, which might be the intended behavior
There was a problem hiding this comment.
ack, will need to fix that
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
No perf impact as expected
This gives us